Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ [rtl] add optional SPI data FIFO #381

Merged
merged 28 commits into from
Aug 2, 2022
Merged

✨ [rtl] add optional SPI data FIFO #381

merged 28 commits into from
Aug 2, 2022

Conversation

stnolting
Copy link
Owner

@stnolting stnolting commented Jul 31, 2022

This PR adds an optional data FIFO to the SPI module (idea by @akaeba from #378). A new top generic is added to configure the SPI FIFO size: IO_TRNG_FIFO, default = 0; has to be zero or a power of two. Furthermore, all FIFO status signals are added to the SPI control register and a fine grained interrupt configuration is added:

  • interrupt if SPI PHY completes the current transmission (default, behavior of pre-PR version)
  • interrupt if SPI TX FIFO becomes empty
  • interrupt if SPI TX FIFO becomes less than half full

✔️ The changes from this PR are fully backwards compatible.

@stnolting stnolting added enhancement New feature or request HW Hardware-related labels Jul 31, 2022
@stnolting stnolting self-assigned this Jul 31, 2022
@stnolting stnolting linked an issue Jul 31, 2022 that may be closed by this pull request
@akaeba
Copy link
Collaborator

akaeba commented Aug 1, 2022

Hi Stephan, i adjusted the driver see #382. Nevertheless there are some bugs inside. For the case IO_SPI_FIFO = 0 is all fine. But if i set IO_SPI_FIFO=32 and run following code example:

while ( 1 ) {
  memset(uint8MemBuf, 0, sizeof(uint8MemBuf)/sizeof(uint8MemBuf[0]));
  uint8MemBuf[0] = 0x03;
  neorv32_spi_rw(&g_neorv32_spi, uint8MemBuf, 0, sizeof(uint8MemBuf)/sizeof(uint8MemBuf[0]), 1);
  while ( neorv32_spi_rw_busy(&g_neorv32_spi) ) {
    __asm("nop");
  }
  while ( 1 ) {
    __asm("nop");
  }
}

image

Then Performs the SPI core with disabled CSn an additional Access.

One other Hint, if the FIFO=0, should tx_fifo.empty the same like busy. This relaxes the SW because you need to to change the config from tx_fifo.empty IRQ to Idle IRQ. Because it's the same.

@stnolting
Copy link
Owner Author

Hi Stephan, i adjusted the driver see #382.

Great! I'll have a look later.

But if i set IO_SPI_FIFO=32 and run following code example: [...] Then Performs the SPI core with disabled CSn an additional Access.

I do not understand your code yet but I will have a closer look at this. 😉

One other Hint, if the FIFO=0, should tx_fifo.empty the same like busy. This relaxes the SW because you need to to change the config from tx_fifo.empty IRQ to Idle IRQ. Because it's the same.

You are right, but I was thinking about this the other way around: basically, software should only check the SPI control register's busy flag. If FIFO_SIZE = 0 this flag indicates (when set) that the SPI engine is currently doing a transmission (full backwards compatible). If FIFO_SIZE > 0 this flag is set when the SPI engine is busy OR when the TX FIFO has not run empty yet. So even software that was not written for handling the FIFO can still rely on the bus flag.

The FIFO status signals should only be used by functions (or IRQ handlers) that were explicitly designed to handle "block transfers" (actually using the FIFO).

@akaeba
Copy link
Collaborator

akaeba commented Aug 1, 2022

I do not understand your code yet but I will have a closer look at this. 😉

I try to write 10 Bytes to the SPI, the core performs also this transaction. But Later when all is done, i see one additional transfer. Thanks for checking :)

@akaeba
Copy link
Collaborator

akaeba commented Aug 1, 2022

The FIFO status signals should only be used by functions (or IRQ handlers) that were explicitly designed to handle "block transfers" (actually using the FIFO).

Okay, i think there i need to explain a little more. If you have FIFO=0, and setup the SPI IRQ=3d, then the Core will not produce IRQs. My segustions is: that in that case the busy irq is also produced even if tx empty IRQ is selected.

@stnolting
Copy link
Owner Author

stnolting commented Aug 1, 2022

I try to write 10 Bytes to the SPI, the core performs also this transaction. But Later when all is done, i see one additional transfer. Thanks for checking :)

I just simulated the code from #382. There are exactly ten transfers:

grafik

SPI signals in light blue. There are 8 transfers between the two cursors; in this time CS (spi_csn_o) is low. The "start" signal just beneath the light blue "busy" signal shows 8 spikes - so 8 transfers are getting triggered in total.

@stnolting
Copy link
Owner Author

stnolting commented Aug 1, 2022

If you have FIFO=0, and setup the SPI IRQ=3d, then the Core will not produce IRQs.

Right. I need to modify the SPI control register so that writing interrupt configurations 10 and 11 (both requiring the FIFO) always falls back to 00 (just checking the busy state of the SPI engine itself) if FIFO_SIZE = 0.

My segustions is: that in that case the busy irq is also produced even if tx empty IRQ is selected.

Good point, but I think having the fall-back option should also resolve this.

Btw, I think software should check if the SPI FIFO is implemented at all (there are 4 bits in the control register that show log2(FIFO_SIZE)) before configuring the SPI module (especially the IRQ configuration).

* clean-up
* add fall-back for interrupt configuration if FIFO size is zero
@stnolting stnolting marked this pull request as ready for review August 1, 2022 15:23
@akaeba
Copy link
Collaborator

akaeba commented Aug 1, 2022

I just simulated the code from #382. There are exactly ten transfers:

I have an idea, I need to check.

@akaeba
Copy link
Collaborator

akaeba commented Aug 1, 2022

Good point, but I think having the fall-back option should also resolve this.

I'll try 😉

stnolting and others added 2 commits August 1, 2022 20:44
…is 'neorv32_spi_rw' interrupted by ISR and after finishing ISR continued which leads the false writes on SPI bus.
@akaeba
Copy link
Collaborator

akaeba commented Aug 2, 2022

Hi Stephan, with f9d8ece is the driver operable. the root cause was the interruption of the data write loop in the '_rw' function by the ISR. Now is the driver operable.

@stnolting
Copy link
Owner Author

Awesome!

self->uint32Fifo = (uint32_t) neorv32_spi_get_fifo_depth(); // acquire FIFO depth in elements

That looks really good! 😉

I will test your code in a simulation later today. If everything looks fine we can merge #382 I think, right?

@akaeba
Copy link
Collaborator

akaeba commented Aug 2, 2022

Perfect, thanks for adding the FIFO. 😉

@stnolting
Copy link
Owner Author

Your modification of demo_spi_irq works as expected, so I think we can merge that. 👍

However, using the "FIFO gets empty" interrupt is not really efficient here as only a single word is written to the FIFO on each IRQ. A more sophisticated approach should make use of "double buffering" utilizing half of the FIFO. But this is something we could add in the future... 😉

thanks for adding the FIFO

You're welcome! And thanks for the idea!

[sw example] demo_spi_irq can handle FIFO
@akaeba
Copy link
Collaborator

akaeba commented Aug 2, 2022

"FIFO gets empty" interrupt is not really efficient here as only a single word is written

I think not directly, lets make a small deep dive. In this function:

int neorv32_spi_rw(t_neorv32_spi *self, void *spi, uint8_t csn, uint32_t num_elem, uint8_t data_byte) {

uint32Buf |= ((uint8_t *) self->ptrSpiBuf)[0];

is only written one data word. This is caused that this function can be interrupted by the ISR. If i try to write more data words then i will mess up the whole packet.

In the ISR there should be written more datawords caused by:

for ( ; self->uint32Write<uint32Lim; (self->uint32Write)++ ) {

because there is no return otherwise the data is send. But your right at the beginning fires the controller more IRQs. If the loops goes on the IRQ rate reduces. This is caused that in the higher speed modes the SPI is really fast. If you like, you can try to set-up a lower speed, then you should see the the FIFO is fully filled by the ISR without any new IRQ.

This knowledge would also enables a completly different driver architecture: you bang out the bytes until the SPI return with busy then you leave the ISR. But the FIFO is much better :)

@stnolting
Copy link
Owner Author

int neorv32_spi_rw(...)
is only written one data word. This is caused that this function can be interrupted by the ISR. If i try to write more data words then i will mess up the whole packet.

Right. Since this is the setup initial function you could disable the SPI interrupt temporarily to fill up the FIFO as much as possible.

But your right at the beginning fires the controller more IRQs. If the loops goes on the IRQ rate reduces. This is caused that in the higher speed modes the SPI is really fast. If you like, you can try to set-up a lower speed, then you should see the the FIFO is fully filled by the ISR without any new IRQ.

You are right! I keep forgetting that this runs in "highspeed" mode... 😅

Anyway, I think this is ready to be merged! 👍

@stnolting stnolting merged commit 860a146 into main Aug 2, 2022
@stnolting stnolting deleted the add_spi_fifo branch August 2, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request HW Hardware-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Idea]: add configurable FIFO to SPI core
2 participants